fix: 대학교 중복 오류 수정 #510
fix: 대학교 중복 오류 수정 #510
Conversation
- findAllByRegionCodeAnKeywords 메서드에서 term을 고려하여 조회하도록 수정
lsy1307
requested review from
Gyuhyeok99,
nayonsoso,
whqtker and
wibaek
as code owners
Walkthrough
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java (1)
34-52: 수정 필요 — term이 null/blank인 경우 필터 누락 방지: termEq 헬퍼 사용으로 일관화하세요.위치: src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java (findAllByRegionCodeAndKeywords, 약 라인 34–52); 파일에 termEq 헬퍼가 이미 정의·사용됨(라인 93, 159).
- 변경 — termEq으로 대체.
return queryFactory .selectFrom(univApplyInfo) .join(univApplyInfo.university, university).fetchJoin() .join(university.country, country).fetchJoin() .leftJoin(univApplyInfo.languageRequirements, languageRequirement).fetchJoin() - .where( - regionCodeEq(country, regionCode) - .and(countryOrUniversityContainsKeyword(country, university, keywords)) - .and(univApplyInfo.term.eq(term)) - ) + .where( + regionCodeEq(country, regionCode), + countryOrUniversityContainsKeyword(country, university, keywords), + termEq(univApplyInfo, term) + ) .distinct() .fetch();
검증 — 단위 테스트 추가로 확인.
term이 null, 빈 문자열, 정상값인 케이스를 분리해 필터링 결과(중복/누락 포함)가 기대대로 나오는지 확인하세요.권고(선택) — languageRequirements의 fetchJoin 검토.
languageRequirements는 to‑many이므로 fetchJoin이 중복/성능 이슈를 만들 수 있으니 필요 시 별도 조회나 배치 사이즈로 조정하세요.
🧹 Nitpick comments (1)
src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepository.java (1)
9-14: 시그니처 확장 검증 완료 및 네이밍 통일 권장
- 시그니처 확장 검증
모든 호출처가 새 파라미터(term 포함)를 올바르게 전달하고 있음을 확인했습니다.- 네이밍 통일 권장
인터페이스(countryKoreanNames)와 구현(countryCodes)의 파라미터명이 달라 혼선 우려가 있으니 일관된 이름으로 변경하세요.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepository.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java
Outdated
Show resolved
Hide resolved
lsy1307
requested review from
Hexeong,
JAEHEE25 and
sukangpunch
as code owners
Gyuhyeok99
left a comment
Gyuhyeok99
left a comment
There was a problem hiding this comment.
변경사항에 대한 테스트코드가 생기면 좋겠습니다!
예를 들어 이전 학기 대학 리스트는 응답에서 제외된다 등이요!
나머진 간단한 코드 개선 의견만 남겼습니다!
| public interface UnivApplyInfoFilterRepository { | ||
|
|
||
| List<UnivApplyInfo> findAllByRegionCodeAndKeywords(String regionCode, List<String> keywords); | ||
| List<UnivApplyInfo> findAllByRegionCodeAndKeywords(String regionCode, List<String> keywords, String term); |
| SiteUser siteUser = siteUserRepository.findById(siteUserId) | ||
| .orElseThrow(() -> new CustomException(USER_NOT_FOUND)); | ||
| List<UnivApplyInfo> univApplyInfos = universityFilterRepository.findAllByRegionCodeAndKeywords(regionCode, List.of(keyword)); | ||
| List<String> keywords = (keyword == null || keyword.isBlank()) ? List.of() : List.of(keyword); |
There was a problem hiding this comment.
List<String> keywords = StringUtils.hasText(keyword) ? List.of(keyword) : List.of();
List<String> keywords = StringUtils.isNotBlank(keyword) ? List.of(keyword) : List.of();
이거 중 하나로 해서 좀더 간결하게 표현하는 건 어떨까요?
|
추가로 사소하지만 pr명 로 수정해주실 수 있을까요? |
- 메서드명에 Term 추가 - 컨벤션 수정
수정했습니다! 그리고 지금 보니 application쪽 테스트 코드는 좀 전반적인 수정이 필요할 것 같아서 다른 pr에 추가하겠습니다! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java(2 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepository.java(1 hunks)src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: nayonsoso
PR: solid-connection/solid-connect-server#486
File: src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java:166-178
Timestamp: 2025-08-26T05:00:56.556Z
Learning: UnivApplyInfo의 텍스트 검색 기능에서는 University.koreanName이 아닌 UnivApplyInfo.koreanName을 검색 대상으로 사용하는 것이 의도된 설계이다.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
src/main/java/com/example/solidconnection/application/service/ApplicationQueryService.java (1)
49-51: 키워드 가드 덕분에 마음이 놓이네요!
null·blank 키워드를 빈 리스트로 털어내니 이제 NPE 공포에서 해방입니다. 덕분에 호출부가 훨씬 단단해졌어요.
| regionCodeEq(country, regionCode) | ||
| .and(countryOrUniversityContainsKeyword(country, university, keywords)) | ||
| .and(univApplyInfo.term.eq(term)) | ||
| ) |
There was a problem hiding this comment.
term null 처리 누락으로 즉시 예외 위험!
term 값이 null이면 Querydsl이 "eq(null) is not allowed" 예외를 곧장 던져서 /application 조회가 바로 깨집니다. 기존 인터페이스에는 term이 없었기 때문에 호출부에서 관성적으로 null을 넣을 여지가 있고, 설정 누락 시에도 즉시 장애가 나니 사전에 가드가 필요해요. 간단하게 null을 검증한 뒤 쿼리를 빌드하도록 바꿔 주세요.
@@
- return queryFactory
+ if (term == null) {
+ throw new IllegalArgumentException("term must not be null");
+ }
+
+ return queryFactory
.selectFrom(univApplyInfo)
@@
- .and(univApplyInfo.term.eq(term))
+ .and(univApplyInfo.term.eq(term))🤖 Prompt for AI Agents
In
src/main/java/com/example/solidconnection/university/repository/custom/UnivApplyInfoFilterRepositoryImpl.java
around lines 46 to 49, the code calls univApplyInfo.term.eq(term) without
guarding term which causes Querydsl to throw "eq(null) is not allowed" when term
is null; change the query construction to only add the term predicate when term
is non-null (e.g., create a termEq(term) helper that returns the predicate when
term!=null or null otherwise, or conditionally chain the .and(...) only if term
!= null) so the query builder never calls eq(null).
저는 변경사항과 함께 테스트 코드가 같이 추가되는 게 맞다고 생각해서 여기서 간단하게라도 작성하고 리팩토링하면 좋겠습니다..! |
아공 제가 이제야 확인했는데 이전_학기_지원자는_조회되지_않는다가 이미 있어서 중복될 것 같은데 이 메서드를 좀 수정해볼까요? |
- 명칭 변경 - 정확하게 현재 학기만 조회되는지 확인하도록 로직 수정
fix: PostRepository & CommentRepository의 일부 메서드의 정렬 순서 오류 해결 (#522) refactor: 이메일 유니크키 제거 refactor: 엔티티가 BaseEntity를 상속하도록 (#524) fix: BaseEntity 관련 마이그레이션 파일 수정 (#530) feat: 멘토 승격 api 구현 (#532) fix: 대학교 중복 오류 수정 (#510) refactor: 불필요한 로그 삭제 (#543) fix: 운영환경 8081 포트 설정 추가 (#542) fix: dev환경 디비명 변경 (#546) refactor: 불필요한 로그 삭제 (#547) refactor: 학기를 테이블로 관리하도록 변경 (#526) refactor: 모의지원 시 지원한 대학 정보 응답 추가 (#539)
3 participants
관련 이슈
작업 내용
특이 사항
리뷰 요구사항 (선택)